dev/core#2034 Fix paypal standard cancel url and action links in dashboard #18787
Conversation
Add supportsCancelRecurring() and supportsUpdateSubscriptionBillingInfo() to override base methods, as these are not supported by PAYPAL_STANDARD. Intended to fix (i.e. remove) broken links in contact contribution dashboard.
Add supportsChangeSubscriptionAmount() to override base method, as this is only supported by PAYPAL_PRO. Intended to fix (i.e. remove) broken links in contact contribution dashboard.
Corrected logic in supportsUpdateSubscriptionBillingInfo() to match updateSubscriptionBillingInfo() - not supported if not PAYPAL_PRO (vs not supported if PAYPAL_STANDARD).
Remove "cancel" link if cancelRecurring is not supported. Also update URLs to use subscriptionURL() method of payment processor.
Update recurLinks() to remove the "View" option if the user has no permission to view their own contact and is in the contact dashboard. (Link does not work without this permission).
Removed inappropriate link to transaction search (via number of instalments) if user has no permission to access CiviContribute. Link does not work without this permission.
Fixed formatting with recurring contributions so that:
(a) spacing is correct
(b) all literal text is enclosed within {ts} tags so that it can be translated / replaced
(c) zero values are displayed correctly
(d) links are disabled if there is no permission to access the link
|
(Standard links)
|
webmaster-cses-org-uk
changed the title
webmaster-cses-org-uk
changed the title
|
Raised https://lab.civicrm.org/dev/core/-/issues/2129 to cover residual issues described in note. |
|
add to whitelist |
Removed trailing whitespace from lines identified by Jenkins (used different editor!!)
|
Wow - there is a lot in here that needs to be worked through. Probably we are going to have to break this down as some changes are also proposed / agreed elsewhere - eg. #18196 also does But other things like wording changes might need more people to agree. We'll work through it! As a first step I've put up #18790 which will add the unit test on the code that we need to have in place before we start changing it. |
|
Thanks Eileen. Yes I prefer Matt's proposed method in #18196 of only adding links that are required, rather than adding them all then taking them away again (although I'm not sure those changes will address all the issues here). This is more elegant but I didn't want to change too much. Thank you for your support and attention on this - I will await further updates and let me know if there is anything I can do to help. |
|
@webmaster-cses-org-uk - yes - I agree that approach is better but it's not the end of the story |
This change is in civicrm#18787 and I agree it makes sense - it stands alone so I have pulled it out
| @@ -73,15 +73,18 @@ public static function recurLinks($recurID = FALSE, $context = 'contribution') { | |||
| if ($paymentProcessorObj) { | |||
| if ($paymentProcessorObj->supports('cancelRecurring')) { | |||
| unset($links[CRM_Core_Action::DISABLE]['extra'], $links[CRM_Core_Action::DISABLE]['ref']); | |||
| $links[CRM_Core_Action::DISABLE]['url'] = "civicrm/contribute/unsubscribe"; | |||
| $links[CRM_Core_Action::DISABLE]['url'] = $paymentProcessorObj->subscriptionURL(NULL, NULL, 'cancel'); | |||
There was a problem hiding this comment.
@mattwire @webmaster-cses-org-uk this link would be available to backoffice users and to people viewing their own dashboard.
I think the desired link & behaviour might be different - ie the front end user should be taken off the paypal whereas the backoffice user can only do an administrative cancel (which shouldn't be available to the front end user).
I have a feeling the code might be simpler if we didn't have one 'recurLinks' function try to deal with both scenarios
There was a problem hiding this comment.
I agree splitting it is probably better. It would be cleaner and easier to maintain: relying on the free-text $context argument leaves a bit too much to chance I think.
One other thing to consider: among the changes I've proposed, supports("cancelRecurring") will return FALSE for PayPal_Standard. This is because the method is currently defined (in the comments) as whether the processor supports cancellation "in code", which of course PayPal_Standard doesn't. This would mean that no link is present for back-office or front-end users.
I think we may therefore also need to split the "cancelRecurring" operation into two: back-office and front-end, and similarly have two modes for subscriptionURL (back-end and front-end cancel).
There was a problem hiding this comment.
Yes - I think so - maybe we should supportUserCancelRecurring (or supportDonorCancelRecurring)
There was a problem hiding this comment.
I'd second that. Either of those names would be fine. Then for subscriptionURL perhaps we could have a corresponding 'userCancel' (or 'donorCancel')?
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
|
I spent a chunk of time digigng into this today and opened #20210 which addresses the issue of us offering an incorrect paypal link & also adds some unit tests and starts separating the generation of front end & back office links. Some of the issues discussed I can no longer replicate and I think it's time to close this PR (it's also stale) and it can potentially come back in some altered form - although I note your main issue was the paypal cancel url which is fixed in the email & in the dashboard with #20210 merged |
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
This picks up on two of the issues that were blocking on civicrm#18787 and civicrm#18196 (but does not solve all of them). It 1) adds the basis for unit test cover and 2) separates the function for retrieving links for self-service from back office user. I think there is still some jumbling together of these concepts but as long as we are careful to extend the tests as we extend the logic... This fixes what I suspect is an oldish regression whereby the cancel link for paypal std users was wrong
Overview
Further to #18540.
Per https://lab.civicrm.org/dev/core/-/issues/2034 the wrong urls are included in paypal standard emails for cancellation and changing billing details. Broken links are also shown on the contact dashboard (recurring contributions), which also has some formatting errors and other broken links.
Before
Email contains civicrm urls
This is a recurring contribution.
You can cancel future contributions at:
http://dmaster.local/civicrm/contribute/unsubscribe?reset=1&coid=98
Likewise, the contact dashboard recurring contributions section shows the following links:
View - does not work if the front-end user does not have "view my contact" permission (e.g. only allowed access through profiles)
Edit - inappropriate as paypal standard does not support editing contribution amount
Cancel - as above
The contact dashboard also has the following errors:
Incorrect spacing between amount and frequency values.
Zero values in instalments displayed as blank (default PHP representation of 0 when printing).
Instalments link to contributions search does not work if user does not have "access CiviContribute" permission.
After
Email contains paypal urls for paypal standard processor
https://www.sandbox.paypal.com/cgi-bin/webscr?cmd=_subscr-find
View, Edit and Cancel links in dashboard removed as appropriate based on permissions and payment processor supported methods.
Formatting corrected.
Link to instalments search disabled if user does not direct access to CiviContribute.
Technical Details
Overriding the parent class to provide the correct url for the processor.
Overriding the parent class to provide correct supportsCancelRecurring(), supportsUpdateSubscriptionBillingInfo() and supportsChangeSubscriptionAmount() methods.
Updating recurring contribution action links generation to take permissions and "supports" methods into account and also to use latest method of generating URLs.
Fixing UserDashboard template for contributions and updating accordingly to match above changes.
Comments
NOTE: In testing I found that if the front-end user has permission to "view my contact" and clicks 'View' next to a recurring contribution, the dialogs that appear include 'Edit' and 'Delete' links irrespective of permissions. (This is true for both the recurring contribution and associated membership dialogs).
For the recurring contribution dialog, the edit and delete links give an error message (window.alert) but, worryingly, seem to let the front end user actually complete those edit and delete operations for which they should not have permission.
I did not attempt to fix this as my use-case is to disable access to these dialogs entirely, however this seems to be a security risk for which I will raise a separate issue.